-
Notifications
You must be signed in to change notification settings - Fork 4
use ProcessHandle
instead of graalvm substitutions
#24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
@@ -8,13 +8,12 @@ trait LockProcess { | |||
object LockProcess { | |||
class Default extends LockProcess { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this change likely means we could inline/remove the whole LockProcess
trait, as well as change Int
to Long
. However, I choose to maintain bincompat instead and just do the minimum here
Processes.isRunning(pid) | ||
} | ||
ProcessHandle.current().pid().toInt | ||
def isRunning(pid: Int): Boolean = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note that this interface doesnt distinguish "no process with that pid" from "there is a process with that pid but it's not alive".
I wonder if there is a practical usecase for forcefully destroying the latter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexarchambault what do you say about merging it, I see I have the same issue building locally when doing scalacenter/bloop#2355
We could fork it and release under scala center if Alexandre doesn't have time to look at it
Thanks, I was waiting for this issue to show up for you as well :) I shipped a vendored version of However, I've been observing some failures to take down running bloop servers on windows. It can be related to any number of updates done at the same time (I haven't investigated), but it could be beneficial for you to build a native-image and verify that it still works well. |
Ach, do you mean with this patch or before it also? |
After. But I bumped all dependencies at the same time, including bloop, bloop-rifle, graalvm. So I cannot really say if it's related, though it does seem "close" to this change, codepath-wise |
@alexarchambault do you mind if we fork into scalameta? That's probably the org I have most power in 😅 Or into coursier if you feel it's better. |
Btw. for now I moved the repo into scala center org, hopefully this alright. I merged your PR there https://github.com/scalacenter/libdaemon-jvm and will do some testing on windows |
Hey!
I'm trying to use bloop-rifle with GraalVM Native Image version
graalvm-community:21.0.2
, upgrading from the venerable 22.3.1. (confusingly, 21 is newer than 22)There are a few changes, not all of them trivial to fix.
At some point I had things building, but failing at runtime like this (in a native image):
So it seems handling of the
org.graalvm.nativeimage:svm
dependency has changed, you're clearly not supposed to have it on theclasspath
at NI build time. You get a bunch of warnings like this:Accepting it on the class path still doesn't make it work.
I see you've set it as a compile-time dependency in this project, maybe some breakage downstream also plays into this.
I'm sure there is a migration path somehow, but it's really incredibly hard to navigate this GraalVM landscape. Basically no docs, outdated google results.
So I did the easy thing and looked at the usage of
svm
in this project to see if I could remove it, and bingo. Java has APIs for working with pids now, in the form ofProcessHandle
.Preliminary testing shows that it works the same as before for me with Native Image.